src/system/mod.rs (1)

237-250: ⚡ Quick win

Unify tap-name parsing in one shared helper.

brew_tap_name is implemented here and again in src/system/packages/brew/mod.rs. If they drift, config-time tap_url attachment and runtime tapped/core routing will disagree. Please centralize this parser and reuse it in both places.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/system/mod.rs` around lines 237 - 250, The brew_tap_name parsing logic is
duplicated (one implementation in brew_tap_name and another in the Brew package
module); extract a single shared helper (e.g., pub(crate) fn parse_brew_tap_name
or make brew_tap_name pub(crate)) and replace the duplicate implementation in
the packages/brew module to call that centralized function, adjusting visibility
and imports as needed so both callers use the same logic and behavior (retain
the exact current semantics: split on '/', require exactly three parts, treat
"homebrew/core" as None, and return the tap via rsplit_once).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/cli/system/use.rs (1)

56-56: ⚡ Quick win

Import Config for consistency.

install.rs and upgrade.rs both import Config from crate::config, but this file uses the fully qualified path. For consistency and brevity, consider adding Config to the existing import at line 9.

♻️ Proposed fix

-use crate::config::{ConfigPathOptions, Settings, resolve_target_config_path};
+use crate::config::{Config, ConfigPathOptions, Settings, resolve_target_config_path};

Then update line 56:

-        let config = crate::config::Config::get().await?;
+        let config = Config::get().await?;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/system/use.rs` at line 56, The code uses the fully-qualified call
crate::config::Config::get() in use.rs; add Config to the existing imports (so
Config is brought into scope) and then replace the fully-qualified call with
Config::get(). In other words, update the imports to include the Config type
(matching install.rs/upgrade.rs) and change the call site (the expression using
crate::config::Config::get()) to Config::get() to keep style consistent and
concise.

src/system/mod.rs (2)

137-145: ⚡ Quick win

Make attach_brew_tap_urls additive.

This helper currently overwrites any pre-set request.tap_url with the config lookup result. If a caller ever passes already-resolved brew requests through it, an unmatched config entry will clear that metadata back to None. Only fill missing values here so the helper behaves like “attach”, not “replace”.

♻️ Suggested change

 pub fn attach_brew_tap_urls(config: &Config, by_mgr: &mut IndexMap<String, Vec<PackageRequest>>) {
     let brew_taps = brew_taps_from_config(config);
     let Some(requests) = by_mgr.get_mut("brew") else {
         return;
     };
     for request in requests {
-        request.tap_url = brew_tap_name(&request.name).and_then(|tap| brew_taps.get(tap).cloned());
+        if request.tap_url.is_none() {
+            request.tap_url =
+                brew_tap_name(&request.name).and_then(|tap| brew_taps.get(tap).cloned());
+        }
     }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/system/mod.rs` around lines 137 - 145, The function attach_brew_tap_urls
currently unconditionally sets request.tap_url from the config which can
overwrite an already-resolved tap_url; change the logic so it only fills missing
values: inside attach_brew_tap_urls (operating on requests from
by_mgr.get_mut("brew")), check request.tap_url.is_none() before calling
brew_tap_name(&request.name) and assigning from brew_taps.get(tap).cloned(),
leaving existing tap_url values intact; keep using brew_taps_from_config and
brew_tap_name as the lookup helpers.

---

359-368: ⚡ Quick win

Add one regression test for tap-url attachment, not just tap parsing.

The new behavior here is the URL attachment path used by mise system use and explicit mise system install, but the added test only exercises brew_tap_name() in isolation. A focused test that proves owner/tap/formula picks up the configured URL while homebrew/core/* stays unset would lock down the cross-file contract more effectively. Based on the install/use call paths in the provided snippets and the PR objective for config-driven tap resolution.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/system/mod.rs` around lines 359 - 368, Add a regression test that not
only asserts brew_tap_name() parsing but also exercises the higher-level tap URL
attachment path used by the CLI commands ("mise system use" / "mise system
install") to ensure owner/tap/formula picks up a configured tap URL while
homebrew/core/* remains unset; in practice add a test which calls the same
install/use code path (the function invoked by those commands) with a configured
tap URL for "railwaycat/emacsmacport/emacs-mac" and assert the resolved
attachment URL is present, then call the same path for "homebrew/core/jq" and
"jq" and assert their attachment URL is None, keeping brew_tap_name() assertions
as part of the test to lock the cross-file contract.